perf(client): improve query queue performance by introducing Deque#3569
perf(client): improve query queue performance by introducing Deque#3569nigrosimone wants to merge 3 commits intobrianc:masterfrom
Conversation
nigrosimone
changed the title
nigrosimone
changed the title
charmander
left a comment
charmander
left a comment
There was a problem hiding this comment.
There was a question about removing the query queue entirely at one point; I’ve always discouraged its use, but I can’t remember if it has any bad behaviours remaining beyond this performance issue and what was addressed with the introduction of _errorAllQueries. I guess there’s the complexity of managing timeouts and client state as it leaves and returns to the pool…. Is it how we want to implement pipelining in the future?
Note: The pool-level queue is separate, and is the queue that most people will be using in practice. (Surprisingly, it’s still on an array + shift implementation too.)
push,shift, andremoveare O(1), reducing overhead and GC pressure.
This remove doesn’t look O(1). It also looks like the queue leaks memory in the form of undefined-valued properties as long as it’s never empty.
Reusing a preexisting implementation like double-ended-queue would be nice unless we’re going to implement a faster remove. In the case when the timeout is the same for all queries, removes will always be at the beginning of the queue anyway… but supporting random cancellation and timeouts (with AbortSignal) at both the pool and client level is nice.
|
I’ve implemented a deque backed by a doubly‑linked list with WeakMap index, so push, shift, and remove are all O(1). This avoids the memory leaks from array holes (undefined) and reduces GC pressure. If the goal is to eventually remove the client queue entirely, this can still serve as a drop‑in improvement in the meantime. I agree that reusing a proven library like double-ended-queue is an option, but since we only need a small subset of operations, a lightweight internal implementation may be easier to maintain. Also, I want to thank you for all the work you put into this project, I really appreciate the effort and dedication you’ve invested in maintaining it. Please also feel free to close the PR if you don’t think it’s interesting or worth pursuing further. |
|
Thanks for the kind words @nigrosimone and sorry for the delay.
|
3 participants
This PR replaces the internal
_queryQueuearray inClientwith a customDequedata structure. Previously, the client relied onArray.shift()andArray.splice()to manage queued queries, which are O(n) operations and become costly under heavy load.With the new Deque:
push,shift, andremoveare O(1), reducing overhead and GC pressure._errorAllQueriesand query timeout handling now useDeque’sforEach,clear, andremovemethods.Benefits: